Fix multiple `expect` attribs in impl block #114417

chinedufn · 2023-08-03T13:58:46Z

Closes #114416

rustbot · 2023-08-03T13:58:54Z

r? @oli-obk

(rustbot has picked a reviewer for you, use r? to override)

chinedufn · 2023-08-03T13:59:17Z

So far I have a failing test (apologies for triggering a CI run).

I'm still looking into how to get it passing. Any tips would be appreciated.

To run the new test:

./x test tests/ui/lint/rfc-2383-lint-reason/expect_unused_inside_impl_block.rs

fmease · 2023-08-04T18:38:50Z

Some rough mentoring instructions tying in with my comment over at the issue:

look into where & how #[expect] is implemented in the compiler (look through the PR(s) that are linked from the relevant tracking issue or ones that you can find by filtering the merged PRs or grep through compiler/ (keyword expect or fragments of the diagnostic messages emitted by #[expect]))
if it's the case that rustc only considers the first primary span, consider updating the lint code to respect all primary spans of a (buffered) lint diagnostic (see MultiSpan)

If you have concrete questions, ask them on Zulip in the help stream (or PM me there as a last resort, though note that I've been hacking on the compiler for merely one year)

chinedufn

Thanks for the tips.

I started by just poking around and figuring out the simplest (but potentially naive) way to get the test passing.

I can try to take a look at the tips about the expect PRs and MultiSpan to see if I can figure out a better way to solve the problem.

Or if this way is good then I'm happy to rebase into a single commit and land as is.

compiler/rustc_errors/src/lib.rs

Closes rust-lang#114416

compiler/rustc_errors/src/lib.rs

compiler/rustc_passes/src/dead.rs

tests/ui/lint/rfc-2383-lint-reason/expect_unused_inside_impl_block.rs

cjgillot · 2023-08-22T17:27:20Z

You patch does not compile. Could you fix it and push again?

chinedufn · 2023-08-22T18:34:48Z

I haven't finished addressing your feedback yet. I was planning to work on this within the next week or two.
If this is more urgent for some reason let me know and I can get back to work on this tonight or tomorrow?

This is my first contribution so please let me know if this delay is a problem or if you have any other feedback around how I should approach this patch.

cjgillot · 2023-08-22T20:11:15Z

There is no rush. I was just getting on this PR. I saw some good progress, but I did not know what to do of it as CI fails.
Please issue @rustbot ready once you get to it.

chinedufn · 2023-09-11T22:17:49Z

Update:

I have the ui test passing locally. Just need to add an incremental test. Then I'll push and use @rustbot ready.

chinedufn · 2023-09-12T07:14:40Z

tests/incremental/issue-114416-expect-unused-inside-impl-block.rs

@@ -0,0 +1,32 @@
+// revisions: rpass1


Seems like it would be nice if it was possible to add revisions: rpass1 to a UI test so that we don't have to duplicate tests like this one across tests/incremetal and tests/ui.

First, is there already a good way to avoid this duplication?

If not, would being able to drive an incremental test from a UI test be undesirable for some reason?

If desirable, I'd be interesting in adding support for using the revisions: rpass1 annotation in a UI test. Do you have any tips on how I might best explore that (in a separate PR)?

See what tests/ui/dep-graph files do. They have an // incremental directive for this.

chinedufn · 2023-09-12T07:21:39Z

compiler/rustc_passes/src/dead.rs

+        assert!(dead_codes.iter().skip(1).all(|id| {
+            let hir_id = tcx.hir().local_def_id_to_hir_id(*id);
+            let level = tcx.lint_level_at_node(lint::builtin::DEAD_CODE, hir_id).0;
+            level == first_lint_level
+        }));


This warn_multiple_dead_codes method seems like it would be a fairly cold code path (as in, you'd have to have a project with a lot of dead code to hit this method a lot), so it seemed fine to add another loop here.

chinedufn · 2023-09-12T07:30:10Z

All feedback addressed. Thank you for the guidance.

The PR guide doesn't mention anything about rebasing my commits https://rustc-dev-guide.rust-lang.org/contributing.html#opening-a-pr. I was planning to rebase into a single commit after this PR was approved. Let me know if that's unnecessary.

@rustbot ready

cjgillot · 2023-09-12T11:36:03Z

compiler/rustc_passes/src/dead.rs

@@ -1,4 +1,4 @@
-// This implements the dead-code warning pass. It follows middle::reachable
+// This implements the dead-code warning pass. It follows crate::reachable
 // closely. The idea is that all reachable symbols are live, codes called


Is it still true?

cjgillot · 2023-09-13T16:45:29Z

compiler/rustc_passes/src/dead.rs

+    // This is because we emit a multi-spanned lint using the lint level of the `dead_codes`'s
+    // first local def id.
+    // Prefer calling `Self.warn_dead_code` or `Self.warn_dead_code_grouped_by_lint_level`
+    // since those methods group by lint level before calling this method.
    fn warn_multiple_dead_codes(


This method needs to be renamed, so it's not used again. lint_at_single_level?

cjgillot · 2023-09-13T16:46:46Z

compiler/rustc_passes/src/dead.rs

    }

-    fn warn_dead_fields_and_variants(
+    fn warn_dead_code_grouped_by_lint_level(


And this one can be named warn_multiple.

cjgillot · 2023-09-13T16:47:00Z

compiler/rustc_passes/src/dead.rs

+                    let hir = tcx.hir().local_def_id_to_hir_id(def_id);
+                    let level = tcx.lint_level_at_node(lint::builtin::DEAD_CODE, hir).0;
+
+                    dead_items.push(DeadVariant { def_id, name, level })


DeadVariant needs to be renamed.

Not sure what the right name would be for this. I renamed it to DeadItem.

cjgillot · 2023-09-13T16:48:42Z

compiler/rustc_passes/src/dead.rs

+                    let hir = tcx.hir().local_def_id_to_hir_id(def_id);
+                    let level = tcx.lint_level_at_node(lint::builtin::DEAD_CODE, hir).0;


Suggested change

let hir = tcx.hir().local_def_id_to_hir_id(def_id);

let level = tcx.lint_level_at_node(lint::builtin::DEAD_CODE, hir).0;

let hir_id = tcx.hir().local_def_id_to_hir_id(def_id);

let level = tcx.lint_level_at_node(lint::builtin::DEAD_CODE, hir_id).0;

cjgillot · 2023-09-13T16:51:10Z

tests/incremental/issue-114416-expect-unused-inside-impl-block.rs

@@ -0,0 +1,32 @@
+// revisions: rpass1


See what tests/ui/dep-graph files do. They have an // incremental directive for this.

chinedufn · 2023-10-02T13:00:01Z

@rustbot ready

cjgillot · 2023-10-04T18:54:08Z

@bors r+

bors · 2023-10-04T18:54:54Z

📌 Commit bf4df06 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-10-04T20:44:42Z

⌛ Testing commit bf4df06 with merge 2bbb619...

bors · 2023-10-04T22:31:39Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 2bbb619 to master...

rust-timer · 2023-10-05T01:35:06Z

Finished benchmarking commit (2bbb619): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 623.925s -> 623.37s (-0.09%)
Artifact size: 272.03 MiB -> 272.00 MiB (-0.01%)

rustbot assigned oli-obk Aug 3, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 3, 2023

chinedufn mentioned this pull request Aug 3, 2023

Regression when using #[expect(unused)] more than once in an impl block #114416

Closed

oli-obk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 3, 2023

This comment has been minimized.

Sign in to view

chinedufn commented Aug 7, 2023

View reviewed changes

compiler/rustc_errors/src/lib.rs Outdated Show resolved Hide resolved

chinedufn force-pushed the fix-expect-unused-in-impl-block-rust-issue-114416 branch from 81c650f to 3f0eb7d Compare August 8, 2023 11:51

Fix multiple expect attribs in impl block

b56a049

Closes rust-lang#114416

chinedufn force-pushed the fix-expect-unused-in-impl-block-rust-issue-114416 branch from 3f0eb7d to b56a049 Compare August 8, 2023 12:34

cjgillot reviewed Aug 8, 2023

View reviewed changes

compiler/rustc_errors/src/lib.rs Outdated Show resolved Hide resolved

compiler/rustc_passes/src/dead.rs Outdated Show resolved Hide resolved

tests/ui/lint/rfc-2383-lint-reason/expect_unused_inside_impl_block.rs Show resolved Hide resolved

cjgillot self-assigned this Aug 8, 2023

Misc progress

8ad46b4

This comment has been minimized.

Sign in to view

Tests passing

7db9c3c

chinedufn commented Sep 12, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 12, 2023

This comment has been minimized.

Sign in to view

Fix formatting

feb8002

chinedufn force-pushed the fix-expect-unused-in-impl-block-rust-issue-114416 branch from 68405d4 to feb8002 Compare September 12, 2023 12:03

cjgillot reviewed Sep 13, 2023

View reviewed changes

cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 16, 2023

Address misc feedback

67379c4

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 2, 2023

This comment has been minimized.

Sign in to view

Tidy

9bb55a0

This comment has been minimized.

Sign in to view

Tidy

bf4df06

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 4, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 4, 2023

bors merged commit 2bbb619 into rust-lang:master Oct 4, 2023
11 checks passed

rustbot added this to the 1.75.0 milestone Oct 4, 2023

chinedufn deleted the fix-expect-unused-in-impl-block-rust-issue-114416 branch October 4, 2023 23:59

pvdrz mentioned this pull request Oct 6, 2023

Fix divergence from upstream master ferrocene/ferrocene#23

Merged

matthiaskrgr mentioned this pull request Dec 24, 2023

ICE: assertion failed: dead_codes.iter().skip(1).all(|id|.... #119267

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix multiple `expect` attribs in impl block #114417

Fix multiple `expect` attribs in impl block #114417

chinedufn commented Aug 3, 2023

rustbot commented Aug 3, 2023

chinedufn commented Aug 3, 2023 •

edited

Loading

This comment has been minimized.

fmease commented Aug 4, 2023 •

edited

Loading

This comment has been minimized.

chinedufn left a comment

This comment has been minimized.

cjgillot commented Aug 22, 2023

chinedufn commented Aug 22, 2023 •

edited

Loading

cjgillot commented Aug 22, 2023

chinedufn commented Sep 11, 2023

chinedufn Sep 12, 2023

cjgillot Sep 13, 2023

chinedufn Sep 12, 2023

This comment has been minimized.

chinedufn commented Sep 12, 2023

This comment has been minimized.

cjgillot Sep 12, 2023

cjgillot Sep 13, 2023

cjgillot Sep 13, 2023

cjgillot Sep 13, 2023

chinedufn Oct 2, 2023

cjgillot Sep 13, 2023

cjgillot Sep 13, 2023

chinedufn commented Oct 2, 2023

This comment has been minimized.

This comment has been minimized.

cjgillot commented Oct 4, 2023

bors commented Oct 4, 2023

bors commented Oct 4, 2023

bors commented Oct 4, 2023

rust-timer commented Oct 5, 2023

		let hir = tcx.hir().local_def_id_to_hir_id(def_id);
		let level = tcx.lint_level_at_node(lint::builtin::DEAD_CODE, hir).0;

Fix multiple expect attribs in impl block #114417

Fix multiple expect attribs in impl block #114417

Conversation

chinedufn commented Aug 3, 2023

rustbot commented Aug 3, 2023

chinedufn commented Aug 3, 2023 • edited Loading

This comment has been minimized.

fmease commented Aug 4, 2023 • edited Loading

This comment has been minimized.

chinedufn left a comment

Choose a reason for hiding this comment

This comment has been minimized.

cjgillot commented Aug 22, 2023

chinedufn commented Aug 22, 2023 • edited Loading

cjgillot commented Aug 22, 2023

chinedufn commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

chinedufn commented Sep 12, 2023

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinedufn commented Oct 2, 2023

This comment has been minimized.

This comment has been minimized.

cjgillot commented Oct 4, 2023

bors commented Oct 4, 2023

bors commented Oct 4, 2023

bors commented Oct 4, 2023

rust-timer commented Oct 5, 2023

Overall result: no relevant changes - no action needed

Fix multiple `expect` attribs in impl block #114417

Fix multiple `expect` attribs in impl block #114417

chinedufn commented Aug 3, 2023 •

edited

Loading

fmease commented Aug 4, 2023 •

edited

Loading

chinedufn commented Aug 22, 2023 •

edited

Loading